Make creative sanitization opt-in and restore creative iframe origin isolation - #956
Make creative sanitization opt-in and restore creative iframe origin isolation#956aram356 wants to merge 9 commits into
Conversation
Allow operators to retain sanitizer-accepted external URLs in POST /auction adm while preserving mandatory server-side sanitization and the existing default behavior.
…isolation Creative sanitization ran unconditionally on every markup bid, stripping `script`/`object`/`embed`/`form` and friends together with their inner content. For script-based creatives — the majority of programmatic display — that leaves nothing renderable, and the slot goes blank with no error: the leftover markup is usually a tracking pixel, so the ad server reports a successful render. Measured over 291 creative deliveries on a live publisher: a median 43% of bytes removed, 29 creatives reduced by more than 80%, and 20 reduced below 500 bytes. One bidder lost 100% of every creative; another lost 76% across 43 of them. Add `auction.sanitize_creatives` so sanitization can be disabled where creatives render in a foreign-origin frame (the Prebid Universal Creative inside the ad server's iframe), and make both creative controls opt-in: `sanitize_creatives` and `rewrite_creatives` now default to false, so a creative ships exactly as the bidder returned it unless a publisher asks for processing. Removing `allow-same-origin` from the creative iframe sandbox is part of the same change rather than a follow-up. Sanitization was documented as "the primary defense against malicious markup", with the sandbox as defense-in-depth — but the sandbox granted `allow-same-origin` alongside `allow-scripts`, which removes its origin isolation entirely. With sanitization now optional, that pairing would leave creative markup able to reach publisher cookies, storage, and same-origin fetches. The two sibling sandboxes (APS_RENDERER_SANDBOX, ADM_IFRAME_SANDBOX) already omit the token for exactly this reason; this brings the third in line, so the origin boundary no longer depends on an optional transform. Note the default change alters behaviour for deployments that never set `rewrite_creatives`: creative URL rewriting is now off unless enabled explicitly. Verified end to end: creatives pass through byte-for-byte (triplelift 8902 -> 8902, openx 22069 -> 22069, previously 100% and 35% losses), page renders with ads serving and no hydration errors.
951799e to
b4f5def
Compare
prk-Jr
left a comment
prk-Jr
left a comment
There was a problem hiding this comment.
Summary
The creative-processing behavior change looks coherent: sanitization and rewriting are independently opt-in, and the creative iframe sandbox no longer combines allow-scripts with allow-same-origin. I did not find blocking issues in the runtime/config changes.
Non-blocking
📝 note
- Update stale auction docs/comments for the new creative-processing defaults:
crates/trusted-server-core/src/auction/README.mdstill describes the old contract: creative HTML is always sanitized, rewriting is enabled by default, and[auction].rewrite_creatives = falseskips rewriting but not sanitization. That is now stale with this PR: bothsanitize_creativesandrewrite_creativesdefault tofalse, and sanitization can be disabled independently. Please update the operator-facing README language aroundcrates/trusted-server-core/src/auction/README.md:252andcrates/trusted-server-core/src/auction/README.md:386to describe the new matrix: default raw bidderadm,sanitize_creatives = truestrips executable markup, andrewrite_creatives = trueapplies proxy/click/runtime rewriting to the current creative HTML.
CI Status
- browser integration tests: PASS
- integration tests: PASS
- integration tests (Fastly EC lifecycle): PASS
- prepare integration artifacts: PASS
- targeted local regression check: PASS (
cargo test --package trusted-server-cli --target aarch64-apple-darwin migrated_legacy_config_applies_rewrite_creatives_environment_override)
ChristianPavilonis
left a comment
ChristianPavilonis
left a comment
There was a problem hiding this comment.
Summary
I found one confirmed blocker in the opaque-origin creative runtime, along with high-risk upgrade and rollback compatibility problems. The sandbox isolation direction is sound, but rewritten clicks can currently fail, existing configurations silently change behavior, and the documented rollback sequence is now unsafe. Please address the inline findings before merge.
51a374d to
e6edc23
Compare
# Conflicts: # crates/trusted-server-cli/tests/config_env_overlay.rs # crates/trusted-server-core/src/auction/formats.rs # crates/trusted-server-core/src/auction_config_types.rs # crates/trusted-server-core/src/config_payload.rs # trusted-server.example.toml
…sanitization-optional Keep both creative-processing controls opt-in (default false) while adopting the base branch's consolidation of auction creative processing into creative::process_auction_creative, which the publisher SSAT/page-bids inline path shares. The shared helper now gates sanitization on sanitize_creatives. Base-side tests that relied on mandatory sanitization or default-true rewriting set the flags explicitly; docs describe the four-mode matrix.
…iants Address review feedback on the opt-in creative-processing change: - Register GET /first-party/proxy-rebuild in every adapter. The creative iframe sandbox no longer grants allow-same-origin, so the click guard's JSON POST preflights with Origin: null and fails; the guard now recovers by navigating to the GET endpoint, which the core handler answers with a 302 chain that CORS does not apply to. - Make the click guard detect an opaque origin up front and skip the doomed POST, going straight to the GET navigation fallback. Dynamic resource signing likewise bails out early instead of issuing a request that cannot succeed; a same-origin parent postMessage broker for opaque-origin signing is tracked as a follow-up. - Enforce the 1 MiB per-creative cap independently of the sanitize/rewrite flags so pass-through and rewrite-only modes cannot ship oversized markup. - Cover the remaining processing modes: rewrite-without-sanitize at both the helper and response-converter level, a byte-for-byte pass-through assertion, and oversized-creative rejection in all four modes. - Update stale documentation that still promised mandatory sanitization and default-on rewriting; describe the client sanitizeCreativeHtml helper as validation-only and add a breaking-change changelog entry with the upgrade and rollback sequences.
…sanitization-optional
prk-Jr
left a comment
prk-Jr
left a comment
There was a problem hiding this comment.
Summary
Adds auction.sanitize_creatives, makes both creative-processing controls opt-in, removes allow-same-origin from CREATIVE_SANDBOX_TOKENS, and recovers the resulting opaque-origin click path via a GET /first-party/proxy-rebuild navigation fallback registered in all four adapters.
The sandbox change is the right call and is correctly scoped: the core handler already answered GET with a 302 (proxy.rs:1899) and validates the original tstoken before re-signing, so the new registrations expose no new capability. Every finding from the previous round is genuinely addressed — the 1 MiB cap is hoisted out of the sanitizer into the shared helper, the (false, true) mode is covered at both levels, the pass-through test asserts a real assert_eq! against the bidder input, and the doc sweep is thorough.
Two things are still blocking, one substantive and one about what the docs promise.
Blocking
🔧 wrench
rewrite_creativesdefault flip is unrelated to the bug being fixed (auction_config_types.rs:92): the measurement justifies making sanitization opt-in; flipping rewriting too silently disables first-party proxying, click conversion and creative TSJS injection for every existing deployment, and inverts the base branch'sskip_serializing_ifrollback contract. Recommend the one-line option the PR description already offers — keepdefault_rewrite_creatives() -> true.- Dynamic resource proxying is now permanently off inside creative iframes (
proxy_sign.ts:31):signProxyUrlreturnsnullin an opaque origin,dynamic_src_guard.ts:91falls back to the raw URL, and runtime-injected<img>/<iframe>load direct from third parties. Deferring the postMessage broker is fine; landing it without a tracked issue number and without saying so indocs/guide/creative-processing.mdis not — the guide still advertises the injected runtime's click/resource protections unqualified.
Non-blocking
♻️ refactor
- Redundant flag in
build_bid_map_omits_oversized_adm(publisher.rs:7564): the cap is unconditional now, so the override only removes coverage of the shipped default mode. - Duplicated opaque-origin predicate (
click.ts:152vsproxy_sign.ts:27), with divergent catch-path semantics.
🤔 thinking
- The four-mode matrix omits the cap: both guides describe the default
(false, false)mode as "deliver the creative exactly as the bidder returned it", but a creative over 1 MiB now yields an empty string and theadmis dropped entirely. The CHANGELOG says "subject to the 1 MiB per-creative cap" — worth the same clause indocs/guide/configuration.mdanddocs/guide/creative-processing.md, since those tables are what operators read when choosing a mode.
⛏ nitpick
- Two "sanitized, non-executable" claims survived the doc sweep, both in security-relevant comments and neither reachable by an inline comment since the lines are untouched:
crates/trusted-server-js/lib/src/core/render.ts:177— "Construct a sandboxed iframe sized for sanitized, non-executable creative HTML."crates/trusted-server-js/lib/src/core/request.ts:82— "Render a creative by writing sanitized, non-executable HTML into a sandboxed iframe."
auction_config_types.rs:30still documents the field as rewriting sanitized HTML.
👍 praise
- The cap moved to the right layer. Treating the 1 MiB limit as a delivery invariant in
process_auction_creative_with_rewriterrather than a sanitizer side effect is the correct fix, andprocess_auction_creative_rejects_oversized_markup_in_every_modepins it across all four combinations instead of just the new one. - The pass-through test proves the actual contract.
assert_eq!(adm, original)at both the helper and response-converter level is what makes "ships exactly as the bidder returned it" a testable claim rather than a marker check. - Opaque-origin recovery is covered end to end — unit tests asserting
fetchis never called, plus GET route registration verified per adapter (including a dedicated Spin route test) rather than assumed from the core handler.
Verified non-issues
</script>breakout from raw bidder markup into the publisher origin: blocked byhtml_escape_for_script(publisher.rs:3142), which escapes<,>,&before thebids=JSON.parse("…")inline script.- The GAM direct-replace path already used
ADM_IFRAME_SANDBOXwithoutallow-same-origin, so it is unaffected. enableDebugFromEnv'slocalStorageaccess is alreadytry-wrapped, so the opaque origin'sSecurityErrorwon't take down the click guard at install time.
CI Status
- fmt: PASS
- clippy / cargo check (fastly, axum, cloudflare native + wasm, spin native + wasm): PASS
- rust tests (fastly, axum, cloudflare, spin, ts CLI, cross-adapter parity): PASS
- js tests (vitest) + format-typescript + format-docs: PASS
- integration tests (browser, Fastly EC lifecycle, prepare artifacts): PASS
ChristianPavilonis
left a comment
ChristianPavilonis
left a comment
There was a problem hiding this comment.
Summary
I found seven additional issues in the current head: one security/control-bypass blocker, four high-severity correctness or compatibility failures, and two medium-severity regressions. In particular, rejected creatives can be restored raw from PBS Cache, and the opaque-origin click recovery is broken independently in the browser runtime and in origin-form request handling. All findings are inline.
…covery Address the second review round: - Keep rewrite_creatives defaulting to true. The measurement behind this PR justifies making sanitization opt-in; flipping rewriting too would silently disable first-party proxying for every existing deployment and invert the rollback serialization contract. Only sanitize_creatives (default false) is new behavior. - Suppress the PBS Cache fallback when a supplied creative is rejected by processing (script-only after sanitize, malformed, or over the 1 MiB cap). The GPT bridge renders the cached bid's original adm, so retaining the hb_cache_* coordinates would deliver exactly the markup processing refused. Cache coordinates now ship only for bids that supplied no creative. - Parse the GET /first-party/proxy-rebuild query from uri().query() instead of the full URI: browsers via the Axum and Spin adapters deliver origin-form URIs, which url::Url::parse rejected before the 302 could be issued. - Strip bidder <base> elements in the rewrite pass itself. Rewriting emits root-relative first-party URLs, and a bidder base href would rebase them onto a third-party origin; sanitization also removes <base> but is independently optional. - Resolve click-guard URLs against a pinned document.baseURI instead of location.href, which is about:srcdoc inside the sandboxed creative iframe and rejects root-relative signed click URLs as a base. Navigation URLs are absolutized the same way. - Never write the GET rebuild fallback into data-tsclick: it is the canonical signed click used to diff later mutation waves. href updates, the canonical source persists, and a second mutation wave still rebuilds. - Share one hasOpaqueOrigin predicate between the click guard and dynamic signing; reference issue #982 (parent postMessage broker) from the code and the creative-processing guide's new runtime-protections note. - Document upgrade sequencing (binary first, then config: old binaries reject blobs carrying sanitize_creatives), the per-mode 1 MiB cap, and the requirement to add both TOML leaves before environment overlays apply; add CLI coverage for the sanitize overlay.
prk-Jr
left a comment
prk-Jr
left a comment
There was a problem hiding this comment.
Summary
Every finding from the previous round is genuinely addressed. build_bid_map now distinguishes absent / accepted / rejected creatives, the GET rebuild parses uri().query() so origin-form request targets work, <base> stripping moved into the shared rewrite_creative_html_impl (so it is independent of sanitization), TRUSTED_BASE_URL pins resolution to document.baseURI, persistRebuiltClick keeps the canonical data-tsclick out of the fallback's reach, rewrite_creatives is back to its default true, and the #982 deferral is tracked in both the code and the guide. The doc sweep, upgrade sequencing, and CLI overlay coverage are all in place.
One question about a behavior change that goes slightly beyond what was asked, plus three non-blocking observations. Nothing blocking.
Blocking
❓ question
hb_cache_*is dropped for accepted creatives, not just rejected ones — the cache-fallback fix also removes the coordinates on the normal accepted path, which are GPT slot targeting keys the Prebid Universal Creative reads directly. Likely intended, but undocumented (crates/trusted-server-core/src/publisher.rs:3259, inline).
Non-blocking
🤔 thinking
- The
about:srcdocbranch is untested — jsdom never exercises it, and the Playwright/chromium harness the deferral note says is missing already runs in CI (crates/trusted-server-js/lib/src/shared/origin.ts:24, inline). - #982's blast radius is narrower than the note implies — dynamic signing only installs under
renderGuard, which defaults tofalse(docs/guide/creative-processing.md:89, inline). - Post-fallback mutation diffing degrades to a warn. Once the opaque-origin fallback is written into
href, every later observer pass canonicalizes/first-party/proxy-rebuild?...against the canonicaldata-tsclick, failsdiffParams, and logsclick base changed; keeping originalat warn level (crates/trusted-server-js/lib/src/integrations/creative/click.ts:236). I traced the loop: it terminates,hrefis not reverted, and the second wave still rebuilds — so this is correct, not a defect. The residual gap is a creative that mutates by appending to the currenthrefrather than replacing it wholesale;rebuilds a second mutation wave after an opaque-origin fallbackreplaces the href outright, so that shape is not covered.
⛏ nitpick
- Spin GET route test asserts only
!= 404while its comment says "the handler 302s" (crates/trusted-server-adapter-spin/tests/routes.rs:521, inline).
👍 praise
- The
Some(adm)/Some("")/Nonetri-state is the right shape for the reported bypass, andassert_no_render_sourcepins both halves — script-only under sanitization and oversized under default settings — rather than only the mode that motivated the fix. <base>stripping went intorewrite_creative_html_impl, shared by the root-relative/auctionpass and the absolute-URL inline pass, with a test at each level. Putting it in the shared impl is what makes "rewriting must not depend on sanitization" structurally true instead of a convention.persistRebuiltClickexpresses the guard positively — "writedata-tsclickonly if this is itself a signed/first-party/click" — rather than blacklisting the rebuild URL. That stays correct if another fallback shape is ever added.proxy_rebuild_get_with_origin_form_uri_redirectsbuilds a real signedtsclickand asserts the 302Locationplus the applied add/del mutations, instead of the weaker "did not 404" check.
Verified non-issues
- The GET branch validates the original
tstokenthroughreconstruct_and_validate_signed_targetbefore re-signing (proxy.rs:1795), so registering GET in every adapter exposes no open redirect. - No creative-bundle module other than
proxy_signdepends on a usable origin;shared/script_guard.ts'slocation.originis not reachable from the creative bundle. enableDebugFromEnv'slocalStorageaccess istry-wrapped, so the opaque origin'sSecurityErrorcannot break guard installation.- The
/auctionpath yields an emptyadmon rejection and carries no cache coordinates, so it has no parallel to thebuild_bid_mapbypass. - The 1 MiB cap is applied after
${AUCTION_PRICE}expansion, so macro-driven growth cannot slip past it.
CI Status
- fmt: PASS
- clippy / cargo check (cloudflare native + wasm32-unknown-unknown, spin native + wasm32-wasip1): PASS
- rust tests (fastly, axum, cloudflare, spin, ts CLI, cross-adapter parity): PASS
- js tests (vitest) + format-typescript + format-docs: PASS
- integration tests (browser, Fastly EC lifecycle, prepare artifacts): PASS
| if !adm.is_empty() { | ||
| )) | ||
| }); | ||
| // Cache endpoint coordinates — only present for PBS bids with |
There was a problem hiding this comment.
❓ question — The cache fallback is now suppressed for accepted creatives too, not just rejected ones
The previous round asked for the PBS Cache fallback to be suppressed when processing rejects a supplied creative. This block also removes hb_cache_host/hb_cache_path whenever a creative is supplied and accepted — the normal PBS path — since only the None arm re-inserts them.
Those two keys are not read solely by the bridge. They are set as GPT slot targeting:
// crates/trusted-server-js/lib/src/integrations/gpt/index.ts:544
TS_BID_TARGETING_KEYS.forEach((key) => {
if (bid[key]) gptSlot.setTargeting(key, String(bid[key]!));
});which is what the Prebid Universal Creative uses to fetch the cached creative on its own. Where the bridge declines to serve — notably the matchedBid.hb_adid !== adId early return at gpt/index.ts:1049, a case this codebase has already hit with non-unique hb_adid — the slot previously still rendered from PBS Cache and now has no render source at all.
The security argument covers this case as well: the cached copy is the raw adm, so serving it would bypass processing whether or not the inline creative was accepted. So this may well be deliberate. If it is, the CHANGELOG entry and PR description should say so — both currently describe only the rejected case:
a supplied creative that processing rejects no longer falls back to PBS Cache coordinates
Could you confirm the accepted-case suppression is intended, and if so extend that sentence to cover it?
| // later injects a `<base>` element cannot redirect resolution (the server-side | ||
| // rewriter also strips `<base>` from creative markup before injecting this | ||
| // runtime). | ||
| export const TRUSTED_BASE_URL: string = (() => { |
There was a problem hiding this comment.
🤔 thinking — The about:srcdoc branch is never executed by any test
The srcdoc fix rests on document.baseURI resolving to the parent document's URL inside a sandboxed srcdoc iframe. Per spec that inheritance holds (an about:srcdoc document's fallback base URL comes from its parent), so I read this as a coverage gap rather than a defect.
But the jsdom tests added in this commit only override window.origin; jsdom's document.baseURI is an ordinary http:// URL, so base !== 'about:srcdoc' is always true and the location.href fallback path never runs. The failure reproduced in headless Chromium last round is therefore still not covered by anything.
The deferral note on the click.ts thread says a real srcdoc regression test is bundled into #982 because both "need the same browser-test harness for creative iframes". A Playwright/chromium harness already runs in CI today:
.github/workflows/integration-tests.yml:156 name: browser integration tests
crates/trusted-server-integration-tests/browser/
So the harness cost may be lower than the deferral assumes — a single test that renders a srcdoc creative with CREATIVE_SANDBOX_TOKENS and asserts a mutated click rebuilds would pin both this and the GET 302 recovery.
| ::: | ||
|
|
||
| This setting does not affect HTML/CSS response rewriting under | ||
| ::: info Runtime protections inside the sandboxed creative iframe |
There was a problem hiding this comment.
🤔 thinking — Worth naming renderGuard here so operators can tell whether #982 affects them
Dynamic resource signing is only installed when the render guard is enabled, and it is off by default:
// crates/trusted-server-js/lib/src/integrations/creative/index.ts:16
const DEFAULT_CONFIG: Required<TsCreativeConfig> = {
clickGuard: true,
renderGuard: false,
};As written, the note reads as though every deployment loses dynamic resource proxying. One clause — that this applies only where renderGuard is enabled — would let an operator skip the rest of the paragraph.
| } | ||
|
|
||
| #[tokio::test(flavor = "multi_thread", worker_threads = 2)] | ||
| async fn first_party_proxy_rebuild_get_is_routed() { |
There was a problem hiding this comment.
⛏ nitpick — The comment promises more than the assertion checks
the route must be registered for GET (the handler 302s)
The test only asserts status != 404, so a 400 or 500 passes — which is the exact weakness called out on the axum registration last round. The real 302 assertion lives in proxy_rebuild_get_with_origin_form_uri_redirects, and routing is genuinely all this test can check cheaply. Either drop the parenthetical or assert 302 with a validly signed tsclick as the core test does.
ChristianPavilonis
left a comment
ChristianPavilonis
left a comment
There was a problem hiding this comment.
Summary
I found six high-severity and three medium-severity issues in the current head. The sandbox-isolation direction is sound, but observer-repaired clicks still lose their mutations at navigation time, Axum cannot complete the new redirect chain, and several common creative shapes or resource types do not work under the opaque-origin design. I reproduced the primary click failure with an isolated Vitest scenario.
All reported CI checks are currently passing, but the test suite does not exercise these end-to-end paths.
| const fallback = buildProxyRebuildUrl(tsClickStr, diff); | ||
|
|
||
| if (typeof fetch !== 'function') { | ||
| if (typeof fetch !== 'function' || hasOpaqueOrigin()) { |
There was a problem hiding this comment.
🔧 P1 — Observer-repaired clicks navigate the stale original URL instead of this fallback
The observer writes /first-party/proxy-rebuild?... to href while deliberately retaining the original signed click in data-tsclick. When the user later clicks, computeFinalUrl() canonicalizes the rebuild URL as a different base and returns the original data-tsclick; guardNavigation() then navigates the unmodified click. Mutations made before user interaction—the primary MutationObserver case—are silently discarded.
I reproduced this with mutate → observer flush → click: window.open received the original /first-party/click?...foo=1, not the rebuild URL carrying bar=2. Preserve a pending rebuild URL separately (for example, a per-anchor WeakMap) and navigate it while keeping the canonical signed click for future diffs. Please add a test that dispatches the click after the observer repair and asserts the actual navigation URL.
| // iframe is an opaque origin (sandbox without `allow-same-origin`), | ||
| // so its JSON POST is blocked by CORS and the guard navigates here | ||
| // for a 302 instead. | ||
| primary_methods: &[Method::GET, Method::POST], |
There was a problem hiding this comment.
🔧 P1 — Axum cannot complete the new GET rebuild redirect chain
The rebuild handler now accepts an origin-form URI and returns a relative /first-party/click?... Location. The browser's second request is also origin-form under Axum, but handle_first_party_click passes req.uri().to_string() to url::Url::parse (proxy.rs:1537, 1964), which requires an absolute URL. The first hop therefore 302s and the second hop fails instead of reaching the advertiser.
Please make the shared signed-target parser accept both absolute-form and origin-form URIs, rather than fixing only the rebuild parser, and add an Axum integration test with a valid signed click that follows both redirects.
| el.remove(); | ||
| Ok(()) | ||
| }), | ||
| // Inject unified tsjs bundle at the top of body once |
There was a problem hiding this comment.
🔧 P1 — Body-less creative fragments never receive the click runtime
Injection occurs only when lol_html observes an explicit <body> token. Common adm fragments such as <a>…</a><script>…</script> contain no body token. The client later wraps the fragment in iframe.html, but that template does not add the creative runtime. With sanitization disabled by default, the fragment script survives and can mutate rewritten links without any observer or guarded click handler, bypassing rebuild recovery and first-party click mediation.
Guarantee runtime injection for rewrite-enabled /auction creatives even when no body token is present—preferably at the client wrapper boundary or with a post-rewrite fallback—and test a script-bearing fragment through the complete response → srcdoc flow.
| // later injects a `<base>` element cannot redirect resolution (the server-side | ||
| // rewriter also strips `<base>` from creative markup before injecting this | ||
| // runtime). | ||
| export const TRUSTED_BASE_URL: string = (() => { |
There was a problem hiding this comment.
🔧 P1 — This base is inherited, not trusted
A srcdoc document inherits the embedding page's base URL, including a cross-origin publisher <base>. Preserved bidder code appearing before the body-injected runtime can also install a base first. The runtime itself is loaded from a root-relative URL (tsjs.rs:8), so it may already be requested from the wrong host before this module executes. Rewritten proxy/click URLs can consequently resolve to an unintended origin, leak their signed query strings, or fail entirely.
Emit the runtime, click, and proxy endpoints as absolute URLs derived from an explicitly trusted request origin; do not derive this security boundary from document.baseURI. Add browser coverage for a cross-origin parent <base> and for bidder code inserting a base before <body>.
| // full pass-through, so oversized markup never reaches rewriting, JSON | ||
| // serialization, or the client. Fail closed with an empty string, matching | ||
| // the sanitizer's own oversized-input behaviour. | ||
| if raw.len() > MAX_CREATIVE_SIZE { |
There was a problem hiding this comment.
🔧 P1 — The 1 MiB cap does not bound the delivered or allocated creative
Only raw input length is checked. Rewriting uses an unbounded output vector, every short URL expands into a signed proxy/click URL, and anchors receive both href and data-tsclick. A sub-1-MiB creative containing many small URL-bearing elements can therefore expand to many times the advertised cap before JSON serialization, creating bidder-controlled CPU and memory amplification in constrained WASM runtimes.
Enforce a bounded rewritten-output size while streaming and reject as soon as the output limit is exceeded. The regression test should use repetitive anchors/resources whose input is below the cap but whose rewritten output exceeds it.
| // defense against malicious markup; the sandbox provides defense-in-depth. | ||
| // viewability measurement, so `allow-scripts` is required for them to render. | ||
| // | ||
| // `allow-same-origin` is deliberately excluded: combined with `allow-scripts` on |
There was a problem hiding this comment.
🔧 P2 — Opaque-origin isolation breaks rewritten resources that require CORS
After removing allow-same-origin, /first-party/proxy is cross-origin from the creative's opaque origin. Proxied responses do not add an opaque-origin-compatible CORS policy. ES modules, fonts, and other CORS-mode resources can therefore be blocked even though the first-party proxy was same-origin before this change. This can blank or partially break the script-based creatives the PR is intended to restore.
Define a safe CORS policy for signed creative-proxy responses (or use another same-origin mediation mechanism) and add real-browser coverage for a proxied module and font whose upstream response lacks permissive CORS headers.
| // fail, or the 1 MiB per-creative cap can trip. `None` = no | ||
| // creative supplied; `Some("")` = supplied but rejected. | ||
| let processed_adm = bid.creative.as_ref().and_then(|raw_creative| { | ||
| if raw_creative.is_empty() { |
There was a problem hiding this comment.
🔧 P2 — An explicit empty adm is misclassified as absent and restores raw cache fallback
The comment immediately above defines Some("") as supplied-but-rejected, but this branch converts it to None. The None arm then emits hb_cache_host and hb_cache_path, allowing the client to retrieve the original cached creative and bypass the processing policy and size cap without a rejection warning. prebid.rs preserves an explicit PBS "adm": "" as Some(""), so this state is reachable.
Reserve None strictly for bid.creative == None; process every Some(raw), including an empty string, into the rejected branch. Add an empty-adm plus cache-coordinates regression test.
| ); | ||
| return String::new(); | ||
| } | ||
| let sanitized = if settings.auction.sanitize_creatives { |
There was a problem hiding this comment.
🔧 P2 — Raw rewrite failures are accepted as partial output
With sanitization disabled, raw bidder input now reaches rewrite_creative_html_impl, but that function discards both rewriter.write() and rewriter.end() errors at lines 848–849 and returns whatever bytes were emitted. The sanitizer handles equivalent failures by rejecting the creative. Parser or memory-limit failures can therefore produce truncated, partially rewritten output that is classified as accepted.
Make rewriting fallible or return an explicit rejected outcome. Do not call end() after a failed write, and never accept partially emitted output. Add a forced rewrite-error regression test.
|
|
||
| ```toml | ||
| [auction] | ||
| sanitize_creatives = false |
There was a problem hiding this comment.
🔧 P2 — Other sections of this guide still describe the old configuration contract
This section is correct, but the architecture flow at line 196 still presents sanitization as unconditional. The full example around line 623 omits sanitize_creatives, and the configuration table around lines 655–658 omits the field while describing rewriting as operating on sanitized markup. Operators can therefore incorrectly believe sanitization is active or copy an example that lacks the TOML leaf required for the documented environment overlay.
Please update the flow, full example, and configuration reference to show both independent controls and their defaults.
3 participants
Fixes #955
Stacks on #916 — targets
feature/optional-creative-rewriting, extending the same "creative processing is opt-in" idea from rewriting to sanitization.Summary
Creative sanitization runs unconditionally on every markup bid, stripping
script/object/embed/formand friends together with their inner content. For script-based creatives — the majority of programmatic display — that leaves nothing renderable, and the slot goes blank with no error: what survives is usually a tracking pixel, soadmis non-empty and the ad server records a normal impression.Measured over 291 creative deliveries on a live publisher: a median 43% of bytes removed, 29 creatives reduced by more than 80%, and 20 reduced below 500 bytes. One bidder lost 100% of every creative; another lost 76% across 43 of them.
Changes
1.
auction.sanitize_creatives— gatessanitize_creative_html, independent ofrewrite_creatives. Lets a publisher disable stripping when creatives render in a foreign-origin frame (the Prebid Universal Creative inside the ad server's iframe — the contextrewrite_creative_htmlalready documents), where the markup cannot reach the publisher origin. Opt-in: defaults tofalse.2.
rewrite_creativeskeeps itstruedefault — an earlier revision of this PR flipped it tofalse; both reviewers asked to keep the base branch's default (the measurement above justifies opt-in sanitization, not disabling rewriting), so only sanitization behavior changes for existing deployments. The new field mirrors the branch'sskip_serializing_ifpattern: defaults are omitted from blobs, non-default values (sanitize_creatives = true,rewrite_creatives = false) are serialized.3.
allow-same-originremoved fromCREATIVE_SANDBOX_TOKENS— in this PR, not a follow-up. The resulting opaque-origin iframe cannotfetchfirst-party endpoints (CORS,Origin: null), so:/first-party/proxy-rebuildnavigation (302 chain), now registered in every adapter and tolerant of origin-form URIs; URL resolution pinsdocument.baseURI(srcdoc inherits the parent URL;location.hrefisabout:srcdoc), and the GET fallback never overwrites the canonicaldata-tsclick;4. Processing invariants hardened — the 1 MiB per-creative cap is enforced in every mode (not just inside the sanitizer); a supplied creative that processing rejects suppresses the PBS Cache fallback (which would otherwise deliver the raw cached
adm); and the rewrite pass strips bidder<base>elements itself, since a base href would rebase the emitted root-relative first-party URLs onto a third-party origin.Why the sandbox change belongs here
Sanitization is documented as "the primary defense against malicious markup", with the sandbox as defense-in-depth. But the sandbox granted
allow-same-originalongsideallow-scripts, which removes its origin isolation entirely — the codebase says so itself onADM_IFRAME_SANDBOX, andAPS_RENDERER_SANDBOXomits the token for the same reason.CREATIVE_SANDBOX_TOKENSwas the only one granting it.With sanitization now optional, that pairing would leave creative markup able to reach publisher cookies, storage, and same-origin fetches. The client-side
sanitizeCreativeHtmlis no backstop — it validates type and emptiness only and returns input unchanged (removedCountis always 0). So the origin boundary moves to isolation, where it no longer depends on an optional transform.Upgrade and rollback
Binaries that predate
sanitize_creativesreject a blob carrying it (deny_unknown_fields), so upgrade the binary first, then push config; the configuration guide documents the sequencing and the unsafe interval. Before rolling back to a binary that predates a field, restore its default and push the default-compatible blob. Environment overlays require both[auction]leaves to exist in the TOML (EdgeZero v0.0.4 cannot create missing leaves).Verification
trusted-server-corelib suite 1762 passed, 0 failed; JS suite 432 passed; CLI suite (incl. new sanitize-overlay coverage) green; all six clippy targets andcargo fmtclean; cross-adapter parity tests pass<base>stripping in both rewrite passes, PBS Cache fallback suppression (script-only / oversized) and retention (absent creative), byte-for-byte pass-through, all four processing modes, oversized rejection per mode, opaque-origin click fallback and two-wave mutation rebuildsanitize disabled, rewrite disabledwith raw == sanitized == output on every creative (8902 → 8902 and 22069 → 22069 for bidders previously losing 100% and 35%), page serving ads with no hydration errors